eslint 9 upgrade #1863
eslint 9 upgrade #1863
Conversation
This upgrades eslint to v9 and switches to a flat config file. This is a breaking change for the eslint configuration. The new configuration is in eslint.config.mjs. This also updates prettier to work with eslint. This results in an eslint configuration that is more in line with eslint and typescript recommendations, but results in a slighlty more strict ruleset being applied. This introduces some initial pain in fixing "working code" but should result in a more consistent codebase.
This commit converts the esbuild.js and build/downloader.ts files to ESM. This removes the need for the ts-node package and allows the files to be run directly with Node.js.
jpogran
force-pushed
the
eslint_9_upgrade
branch
from
20cece0
to
a9dd3b5
Compare
| "vscode:prepublish": "npm run compile:prod", | ||
| "package": "vsce package", | ||
| "pretest": "npm run compile:tests && npm run compile && npm run lint", | ||
| "test": "vscode-test", | ||
| "wdio": "npm run compile && wdio run ./src/test/e2e/wdio.conf.ts", | ||
| "lint": "eslint src --ext ts", | ||
| "prettier": "prettier \"**/*.+(js|json|ts)\"", |
There was a problem hiding this comment.
I'm fine with using prettier for most files in our repository now. We just need to keep this in mind when working with them, like GitHub workflow files. Maybe we should document this somewhere.
There was a problem hiding this comment.
I added markdown to the ignore to prevent touching the docs, but thought since the changes to the yaml were along best practices (tabs, spaces, quoted strings, etc) it was best to include them
| @@ -24,17 +24,20 @@ export let TerraformCloudHost = 'app.terraform.io'; | |||
| export let TerraformCloudAPIUrl = `https://${TerraformCloudHost}/api/v2`; | |||
| export let TerraformCloudWebUrl = `https://${TerraformCloudHost}/app`; | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/require-await | |||
There was a problem hiding this comment.
What do you think about disabling this rule globally? I think our current approach here is most readable, when working with external packages that require async functions.
There was a problem hiding this comment.
It's 5 instances across what I had to change, if it was more then I'd be more inclined to make it global. Awaiting async is a better pattern, no?
| ) { | ||
| this.commands = [ |
There was a problem hiding this comment.
To me, it looks like before the change, all the commands and features took care of their internal subscritions and disposals. Now it looks like we're passing the context around and adding everything to a global?
There was a problem hiding this comment.
In each of these dispose patterned classes eslint warned of "unsafe return of value of type any', and IIRC a possible unhandled exception in a dispose method.
I think we have to examine the command classes and features separately. The features are controlled by the StaticFeature lifetime, which to my understanding of documentation are kinda like singletons. Where as the commands are controlled by 'us', and are supposed to run the lifetime of the extension.
I may be over thinking, but ExtensionContext.subscriptions will dispose at deactivation and so we want all things (like command handlers) to sit until extension is deactivated. Whereas a StaticFeature is registered at LSP initialization and may call it's methods at undetermined times, and the dispose method is not part of StaticFeature's signature so it won't call it, we are relying on things still being "alive" when the extension deactivates.
This may be far more thought than needed for this, but it looked to me like less code that left the disposal time to the framework and not to us.
This upgrades eslint to v9 and switches to a flat config file. This is a breaking change for the eslint configuration. The new configuration is in eslint.config.mjs. This also updates prettier to work with eslint.
This results in an eslint configuration that is more in line with eslint and typescript recommendations, but results in a slightly more strict ruleset being applied. This introduces some initial pain in fixing "working code" but should result in a more consistent codebase.
This also converts the esbuild.js and build/downloader.ts files to ESM. This removes the need for the ts-node package and allows the files to be run directly with Node.js.